Fix colorize_code, fix irb crash. #391

tompng · 2022-08-10T19:58:56Z

Fixed IRB::Color.colorize_code to fix irb crash.

irb crashes when I pasted the code below.

__END__
foobar

/Users/tomoya/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/reline-0.3.1/lib/reline/line_editor.rb:1177:in `+': no implicit conversion of nil into String (TypeError)
	from /Users/tomoya/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/reline-0.3.1/lib/reline/line_editor.rb:1177:in `render_partial'
	from /Users/tomoya/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/reline-0.3.1/lib/reline/line_editor.rb:508:in `rerender'

IRB::Color.colorize_code did not colorized the DATA part, and Reline.output_modifier_proc returned too few lines, caused string + nil error.
IRB::Color.colorize_code must colorize all lines, and IMO, all characters.

Ripper.lex sometimes skips some characters.

# "world" is skipped.
Ripper.lex("hello\n__END__\nworld").map{_3}.join #=> "hello\n__END__\n"
# "\0\0foo\n" and "bar" are skipped.
Ripper.lex("<<A\0\0foo\nA\nbar").map{_3}.join #=> "<<AA\n"

Since colorize_code is used in ruby -run -e colorize, ability to colorize every characters in a code that includes "\0" "\4" "\32" might be useful.
In this pull request, I changed IRB::Color.scan to also scan those skipped chars and include it in the colorized string.

st0012 · 2022-09-16T22:16:13Z

lib/irb/color.rb


        scan(code, allow_last_error: !complete) do |token, str, expr|
+          if token.nil?


When I commented out this entire condition, the tests still passed.
I think it's because the str will still be passed to Reline::Unicode.escape_for_print at line 145 and then be pushed to colored at line 150. So the end result is still the same.

Therefore, this condition doesn't seem to be needed?

It actually works without this change, (I did not notice that) but I think this code is necessary.

I think symbol_state.scan_token(token) and dispatch_seq is originally designed to receive Ripper::Lexer::Elem#event that is not nil.
The current implementation of scan_token and dispatch_seq works just by chance with nil, but I don't think we should pass nil to it.

I see, thanks for the clear explanation 👍

st0012 · 2022-09-16T22:43:09Z

lib/irb/color.rb

+            line_positions << line_positions.last + line.bytesize
+          end
+
+          on_scan = proc do |elem|


Do you think it's more understandable if we rewrite the block like this?

on_scan = proc do |elem| start_pos = line_positions[elem.pos[0] - 1] + elem.pos[1] # yield uncolorable code if byte_pos < start_pos yield(nil, inner_code.byteslice(byte_pos...start_pos), nil) end if byte_pos <= start_pos str = elem.tok yield(elem.event, str, elem.state) byte_pos = start_pos + str.bytesize end end

Reasons for the changes:

It's easier to understand if we always put byte_pos and start_pos at the same position in comparisons.

We can move local variable assignment closer to where it's going to be used.

In complicated logic like this, I feel traditional if block is easier to understand.

The code you suggested looks more readable. thanks

st0012 · 2022-09-16T22:45:50Z

lib/irb/color.rb

            end
          else
-            lexer.parse.each do |elem|
-              yield(elem.event, elem.tok, elem.state)
+            lexer.parse.sort_by(&:pos).each do |elem|


Tests still pass without this change. Why is it needed?

I added test case <<A+1\nA for this.

Another code lexer.parse.reject in ruby-lex.rb should also be fixed to lexer.parse.sort_by(&:pos).reject. This change is in another pull request #390

I removed this sort_by again and the test still passed 🤔

This else branch is only executed when ruby < 2.7 and test failed in my environment ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin19] without sort_by.

[tomoya:irb]% ruby -v ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin19] [tomoya:irb]% git log -n1 commit da54e7f0813d88546091af5bf220d83a6bd10198 (HEAD -> colorize_every_characters, tompng/colorize_every_characters) Author: tompng <tomoyapenguin@gmail.com> Date: Mon Sep 19 14:14:10 2022 +0900 Rewrite on_scan proc to be more readable. [tomoya:irb]% git diff [tomoya:irb]% rake test ...skip... .......................... Finished in 6.179684 seconds. ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 114 tests, 1158 assertions, 0 failures, 0 errors, 6 pendings, 0 omissions, 0 notifications 94.7368% passed ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 18.45 tests/s, 187.39 assertions/s [tomoya:irb]% # delete sort_by(&:pos) [tomoya:irb]% git diff diff --git a/lib/irb/color.rb b/lib/irb/color.rb index 7071696..abe0590 100644 --- a/lib/irb/color.rb +++ b/lib/irb/color.rb @@ -196,7 +196,7 @@ module IRB # :nodoc: on_scan.call(elem) end else - lexer.parse.sort_by(&:pos).each do |elem| + lexer.parse.each do |elem| on_scan.call(elem) end end [tomoya:irb]% rake test ...skip... ..................F ========================================================================================================================================================================= Failure: test_colorize_code(TestIRB::TestColor) /Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:269:in `assert_equal_with_term' /Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:139:in `block in test_colorize_code' 136: 137: tests.each do |code, result| 138: if colorize_code_supported? => 139: assert_equal_with_term(result, code, complete: true) 140: assert_equal_with_term(result, code, complete: false) 141: 142: assert_equal_with_term(code, code, complete: true, tty: false) /Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:137:in `each' /Users/tomoya/Documents/github/ruby/irb/test/irb/test_color.rb:137:in `test_colorize_code' Case: colorize_code("<<A+1\nA", complete: true) Result: "#{RED}<<A#{CLEAR}+1\n#{RED}A#{CLEAR}" <"\e[31m<<A\e[0m+\e[34m\e[1m1\e[0m\n" + "\e[31mA\e[0m"> expected but was <"\e[31m<<A\e[0m+1\n" + "\e[31mA\e[0m"> diff: ? <<A+1m1 A ========================================================================================================================================================================= ...skip... .......................... Finished in 4.4035 seconds. ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 114 tests, 1103 assertions, 1 failures, 0 errors, 6 pendings, 0 omissions, 0 notifications 93.8596% passed ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 25.89 tests/s, 250.48 assertions/s rake aborted! Command failed with status (1) /Users/tomoya/.rbenv/versions/2.6.0/bin/bundle:23:in `load' /Users/tomoya/.rbenv/versions/2.6.0/bin/bundle:23:in `<main>' Tasks: TOP => test (See full trace by running task with --trace)

Ah sorry I missed that 🤦‍♂️

st0012 · 2022-09-16T22:46:26Z

lib/irb/color.rb

            end
          end
+          yield(nil, inner_code.byteslice(byte_pos...inner_code.bytesize), nil) if byte_pos < inner_code.bytesize


Can we add a comment to explain what cases this handles?

I added comment yield uncolorable DATA section

st0012 · 2022-09-21T08:45:31Z

@tompng thanks for your fix, I think this PR looks good now 🙌
@peterzhu2118 can you help me merge it when you have time? Thx

peterzhu2118

Looks good. Thank you for your contribution!

tompng marked this pull request as draft August 10, 2022 20:35

tompng force-pushed the colorize_every_characters branch 2 times, most recently from b242065 to a7efb93 Compare August 10, 2022 21:32

tompng marked this pull request as ready for review August 10, 2022 21:35

st0012 reviewed Sep 16, 2022

View reviewed changes

tompng added 3 commits September 19, 2022 14:23

Scan every single characters in IRB::Color.scan

d14e56a

Update expected colorize result that were uncolored before

52446eb

Rewrite on_scan proc to be more readable.

da54e7f

tompng force-pushed the colorize_every_characters branch from a7efb93 to da54e7f Compare September 19, 2022 05:32

peterzhu2118 approved these changes Sep 21, 2022

View reviewed changes

peterzhu2118 merged commit 13fd986 into ruby:master Sep 21, 2022

tompng deleted the colorize_every_characters branch September 21, 2022 17:37

tompng mentioned this pull request Sep 28, 2022

IRB crashing when using "dot" accessor inside interpolation in heredoc #392

Closed

st0012 mentioned this pull request Oct 3, 2022

Bump version to 1.4.2 #407

Merged

st0012 added the bug Something isn't working label Jan 2, 2023

tompng mentioned this pull request Jun 17, 2023

Fix RubyLex.ripper_lex_without_warning with heredoc and embexpr #608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix colorize_code, fix irb crash. #391

Fix colorize_code, fix irb crash. #391

tompng commented Aug 10, 2022

st0012 Sep 16, 2022

tompng Sep 19, 2022

st0012 Sep 19, 2022

st0012 Sep 16, 2022

tompng Sep 19, 2022

st0012 Sep 16, 2022

tompng Sep 19, 2022

st0012 Sep 19, 2022

tompng Sep 20, 2022

st0012 Sep 20, 2022

st0012 Sep 16, 2022

tompng Sep 19, 2022

st0012 commented Sep 21, 2022

peterzhu2118 left a comment


		scan(code, allow_last_error: !complete) do \|token, str, expr\|
		if token.nil?

Fix colorize_code, fix irb crash. #391

Fix colorize_code, fix irb crash. #391

Conversation

tompng commented Aug 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

st0012 commented Sep 21, 2022

peterzhu2118 left a comment

Choose a reason for hiding this comment